Skip to content

refactor(modal): upgrade to 1.6.3 and use public SDK APIs - #533

Open
thundergolfer wants to merge 2 commits into
scaleapi:mainfrom
thundergolfer:codex/upgrade-modal-1.6.3
Open

refactor(modal): upgrade to 1.6.3 and use public SDK APIs#533
thundergolfer wants to merge 2 commits into
scaleapi:mainfrom
thundergolfer:codex/upgrade-modal-1.6.3

Conversation

@thundergolfer

@thundergolfer thundergolfer commented Nov 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Hey, upgrading Modal to get on our latest backend, improving perf and reliability. Also uses new GA methods, no more experimental 🙂

Raise the Modal client requirement from >=1.5.2.dev22 to >=1.6.3 and update the locked client from 1.6.2.dev1 to the latest stable release. Use the public APIs available with this floor:

  • Create ordinary containers and VMs with Sandbox.create(). Keep _experimental_create(i6pn=True) for private networking, which the public factory does not expose, and preserve GPU restrictions.
  • Select the VM runtime with runtime="vm" instead of experimental_options={"vm_runtime": True}. Preserve the nested-virtualization option for disk sandboxes.
  • Pull ECR images with Image.from_registry() and the existing named AWS secret, preserving credential routing and registry-host checks.

Update the provider tests and extend the installed-SDK signature checks to cover both public container creation and private networking. File-transfer changes are outside this PR.

Regenerated uv.lock with the CI-pinned uv 0.13.1. The only package version changes are Modal and its required synchronicity update (0.12.6 → 0.12.7); uv also normalizes lockfile markers and format.

Validation:

  • Modal provider unit tests: 129 passed, including installed-SDK signature checks, VM/disk creation, GPU restrictions, cancellation cleanup, network policies, and image authentication.
  • Shared attribution, network-policy stamping, and provider-capability tests: 133 passed, 1 skipped (optional smol package unavailable).
  • uv lock --check with uv 0.13.1: passed.
  • uv run --frozen ruff check .: passed.
  • git diff --check: passed.

Live Modal integration tests were not run. Upstream CodeBuild requires maintainer approval before running for this fork.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no actionable new defects were established.

Summary

Raises the Modal requirement to >=1.6.3 and updates the providers to use its public APIs.

  • Modal installs now require the stable 1.6.3 client.
  • Container sandboxes use Modal’s public creation call except for private networking.
  • VM sandboxes use Modal’s public factory and explicit VM runtime setting.

Reviews (2) · Last reviewed commit: "refactor(modal): use public sandbox and ..." · Reviewed by Greptile

@thundergolfer
thundergolfer requested a review from a team as a code owner November 7, 2026 17:57
@socket-security

socket-security Bot commented Nov 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security		 Vulnerability Quality Maintenance License
Updatedpypi/​modal@​1.6.2.dev1 ⏵ 1.6.380 +1100100100100

View full report

@thundergolfer thundergolfer changed the title build(deps): upgrade Modal client to 1.6.3 refactor(modal): upgrade to 1.6.3 and use public SDK APIs Nov 7, 2026
@earakely-scale

earakely-scale commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator

🤖 Automated Claude routine — Env Pod PR Context Review. This is a bot, not Edgar Arakelyan; not a human review.

Offering context on one branch this repo's tests can't exercise end to end: the ECR pull-secret path in _registry_image.

The PR description says ECR images are pulled "with Image.from_registry() and the existing named AWS secret, preserving credential routing". The registry-host check is indeed preserved, but the secret shape looks like it changes meaning. The rewritten call keeps required_keys=ECR_READER_SECRET_KEYS, which is ["AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_REGION"] (src/agent_env/providers/sandbox_providers/modal_sandbox.py:42). The very next branch of the same function shows what this repo expects a from_registry secret to carry — REGISTRY_USERNAME / REGISTRY_PASSWORD (modal_sandbox.py:522-525 on main). So after the change that branch hands from_registry a secret with AWS keys and no registry username/password, and the AWS→ECR token exchange that from_aws_ecr performed no longer has an owner.

Two reasons this may not show up before it ships:

  • The assertions that pinned the old behaviour are relaxed in the same diff — assert aws_ecr.call_args.args[0] == ecr and the aws_ecr.assert_not_called() negative control are both dropped, and the test's docstring ("ECR -> from_aws_ecr + named secret") goes with them. Both factories were previously patched side by side, which is what made the two paths distinguishable; a green suite no longer does.
  • The branch only activates when a deployment sets ecr_pull_secret_name and its image store carries EcrCredentials. Nothing checked into this repo turns that on, so neither the unit suite nor a fork CI run reaches it — and the description notes live Modal integration tests weren't run.

If from_aws_ecr is being retired in 1.6, the AWS→registry credential exchange likely needs to move into the provider (mint a token, pass it as REGISTRY_USERNAME/REGISTRY_PASSWORD) rather than be dropped — the uncached fallback at the bottom of the same function already mints an ECR token, so the pieces are there. Worth a maintainer confirming against a real private-ECR pull before merge.

Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thundergolfer @earakely-scale